Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x11: emit mouse wheel events, drop the ctrl+arrow workaround #303

Closed
wants to merge 1 commit into from

Conversation

tom95
Copy link
Contributor

@tom95 tom95 commented Nov 7, 2018

These changes fixed the horizontal scrolling issues for me. Cursory testing revealed no unintended sideeffects, but more testing should be done. I did not find an easy way to test if horizontal scrolling works as intended, plainly logging the events looks good, however.

No image-side changes were necessary, so I would claim that the VMs for the other platforms can continue to work as normal, even with this change just on x11.

Using the workaround to mark a scroll event by detecting all modifiers being pressed down instead, will require an image-side change though.

@eliotmiranda
Copy link
Contributor

eliotmiranda commented Nov 9, 2018

Hi Tom,

not my area of expertise but I see from platforms/Cross/vm/sq.h this:
/* User input recording II:
   The following functions and definition can be used on
   platform supporting events directly.
*/

/* Event types. */
#define EventTypeNone       0
#define EventTypeMouse      1
#define EventTypeKeyboard   2
#define EventTypeDragDropFiles  3
#define EventTypeMenu       4
#define EventTypeWindow     5
#define EventTypeComplex    6 /* For iPhone apps */
**#define EventTypeMouseWheel 7 /* defunct; platforms map to EventTypeKeyboard */**
#define EventTypePlugin     8 /* Terf: events from ActiveX Controls */

So what /is/ the status of systems w.r.t. genuine mouse wheel events? Should we instead use e.g.

#if DeliverMouseWheelEvents
... your code ...
#else
... existing code ...
#endif

or better still have some variable, settable via a primitive, etc, to enable delivery of mouse wheel events?  The mapping of wheel events to to key presses seems like a massive hack to me but images depend on it, so if we do deliver them as proper events it means either (effectively) a new VM, or the VM providing a way to switch between the two variants.  Since backwards compatibility is key we either make this a #define and remember to enable it at some future date (an approach almost guaranteed to fail from forgetfulness) or we bite the bullet and provide proper modal control (provide both modes and a way of controlling which mode, with a flag that persists in the image so that it doesn't have to be set on every launch).

@tom95
Copy link
Contributor Author

tom95 commented Nov 9, 2018

I have found that the image had good support for MouseWheelEvents for a while. Based on the method stamps it seems Marcel added the necessary image-side code on 6/10/2016 via the MouseWheelEvent and MouseWheelState classes.

The original comment that you pointed out in the opensmalltalk sources comes from commit 3010e44

You're right though that images from before October '16 on X11 would find themselves without scroll support. Do the images have some sort of capability advertising system or will this need to be a primitive invoked on system startup?

I believe this is a rather urgent fix. As Chris already pointed out on the mailing list, it effectively makes scrolling in any function larger than the code panel effectively impossible on X11 in 5.2. Or at least I keep slipping in horizontal scroll events when using my touchpad. If this PR turns out to be a major undertaking, however, we may be better off reverting the commit that enabled horizontal scroll events in the first place for the moment and revisit this afterwards with the cleanest possible solution.

What is the scope of backwards compatibility for the VM? Are breaking changes like this allowed in major version increments (e.g. 5.2->5.3 or 5.2->6.0) or to be avoided at all cost?

@eliotmiranda
Copy link
Contributor

The scope of backward-compatibility for the VM is complete except between major releases, so a 5.n VM must be backward-compatible with any and all 5.x, x <= n. So the right way to do this is
a) make all platforms test a flag (I propose sendWheelEvents) and if the flag is set, deliver the events as EventTypeMouseWheel, otherwise to do the current mapping hack
b) add a bit to the image state flags that persist in the image header that corresponds to the sendWheelEvents, and initialize sendWheelEvents at start-up based on this flag
c) add a command-line switch (eg -wheelevents & -no-wheelevents) to override the flag

Tom, if you have energy can you write code for a) ? I will do b) & c), and will do a) if you're indisposed.

@eliotmiranda
Copy link
Contributor

Closed in favor of 897ef17

@tom95
Copy link
Contributor Author

tom95 commented Nov 10, 2018

I see you were a little faster than me :) Thank you for all the adaptions!

tesonep added a commit to tesonep/opensmalltalk-vm that referenced this pull request Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants